-
Notifications
You must be signed in to change notification settings - Fork 0
Improved overflow support: Overflow auto-handling for directory watches (non-recursive) #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved overflow support: Overflow auto-handling for directory watches (non-recursive) #24
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## improved-overflow-support-main #24 +/- ##
==================================================================
+ Coverage 78.7% 79.7% +0.9%
- Complexity 95 107 +12
==================================================================
Files 12 14 +2
Lines 428 483 +55
Branches 41 46 +5
==================================================================
+ Hits 337 385 +48
- Misses 69 70 +1
- Partials 22 28 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…non-recursive) directory watching, including a test
…ler (depending on the overflow policy)
DavyLandman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is going the direction we discussed, but I have some concerns with the implementation. Mostly small, except for the usage of the Files.walk.
src/main/java/engineering/swat/watch/impl/jdk/JDKRecursiveDirectoryWatch.java
Show resolved
Hide resolved
src/main/java/engineering/swat/watch/impl/overflows/MemorylessRescanner.java
Outdated
Show resolved
Hide resolved
…the `BasicFileAttributes`
…o-handling overflows
sungshik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Davy for the review! I addressed most things, except for the renaming of the enum constants, for which I have another suggestion. Once we converged on those names, I'll also update the top-level docs.
src/main/java/engineering/swat/watch/impl/jdk/JDKRecursiveDirectoryWatch.java
Show resolved
Hide resolved
src/main/java/engineering/swat/watch/impl/overflows/MemorylessRescanner.java
Outdated
Show resolved
Hide resolved
| case NO_RESCANS: | ||
| return eventHandler; | ||
| case MEMORYLESS_RESCANS: | ||
| return new MemorylessRescanner(executor).andThen(eventHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my mind, we should first forward the overflow event, and after that, start the simulation. so a user might be able to recognize simulated events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this could be useful, but it seems quite nontrivial to implement (but I'm maybe overthinking/misunderstanding the idea). Two issues:
- The generation of synthetic events runs asynchronously on the executor, similar to how it currently works in
JDKRecursiveDirectoryWatcher. I think that's in principle a reasonable choice. However, it could mean that between the original overflow event and the synthetic events, concurrent/unrelated "actual" events come in. - Even if there's no interference, there's currently no way of telling how many synthetic events are to be expected.
If we want to support this, then a more reliable approach could be to tag WatchEvent objects as ORIGINAL or SYNTHETIC(i), where i is some kind of an overflow identifier (to be able to distinguish incoming synthetic events that belong to different overflow events, e.g., when they happen shortly after each other). For now, I propose to make a GI issue for this to remember, but not yet implement it, because: (a) it's a cool feature but unclear how much needed; (b) there's already a workaround available, namely, for the programmer to just implement their own overflow handler. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant: chain the simulator after the forwarder, instead of now, first sinulate then forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chain consists of:
MemorylessRescanner: Asynchronously simulates CREATED/MODIFIED events and forwards them toeventHandlereventHandler: User-defined event-handler that does the actual event-handling work
Changing the order isn't needed to achieve the intended effect (also covered by this test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm a bit confused. I mean that from a client perspective I want us to make sure that on an overflow, the overflow events get send to the client first, and only after that is send do we start sending simulated events.
And maybe this is happening already? But when reading the code I thought: should it not be:
return eventHandler.andThen(new MemorylessRescanner(executor))
but I have to admit I'm missing a bit of context of the eventHandler code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, that's what you intend to achieve 🙂! Yes, we can enforce that, but as a client, I don't think it makes a difference, because it can't distinguish simulated events from regular events (see also previous comment), regardless of whether the overflow comes first, last, or anywhere in between. I don't see any harm in your proposed change either, though, so now that I understand the intension behind it, I'll change it and merge.
DavyLandman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final remark left (on the order of the overflow & the syntatic events). But LGRM
This PR makes the following changes:
NONE(do nothing; the user-defined event handler takes care of it),ALL(larger over-approximation, smaller memory footprint), andDIRTY(smaller over-approximation, larger memory footprint). See the documentation ofOnOverflowfor details.NONEandALL. The next PRs will provide an implementation ofDIRTY.PATH_AND_CHILDRENwatch scope. The next PRs will enable it for the other watch scopes.The general design of enabling an overflow policy is captured in the following code fragment:
java-watch/src/main/java/engineering/swat/watch/Watcher.java
Lines 171 to 175 in a28ac94
That is, the actual event handler passed to the
JDKDirectoryWatchconstructor is composed of two separate event handlers:CREATED/MODIFIEDevents (depending on which overflow policy is active);Watcherby the end-user (with a small optimization to ignore overflow events when they're already auto-handled by the policy).